Feat/validate prod deps ci#651
Conversation
annajowang
requested review from
Yuangwang,
abhis3,
falahat,
jamesdaniels,
sjjj986 and
taeold
as code owners
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds environment variable validation for the Angular adapter, implements help flag support for the Angular and Next.js adapter build and create tools, and introduces a new package verification script. Feedback on the verification script recommends using npm pack --json to reliably parse generated tarball filenames and wrapping shell arguments in quotes to safely handle paths containing spaces.
| const output = execSync("npm pack", { cwd: pkgPath, encoding: "utf8" }).trim(); | ||
| // npm pack might output multiple lines if there are warnings, get the last line which should be the tarball name | ||
| const tarballName = output.split("\n").pop().trim(); |
There was a problem hiding this comment.
Parsing the last line of npm pack output to find the tarball filename is fragile and can easily break if npm outputs warnings or other messages to stdout. Using npm pack --json is a much more robust and reliable way to programmatically retrieve the generated tarball filename.
| const output = execSync("npm pack", { cwd: pkgPath, encoding: "utf8" }).trim(); | |
| // npm pack might output multiple lines if there are warnings, get the last line which should be the tarball name | |
| const tarballName = output.split("\n").pop().trim(); | |
| const output = execSync('npm pack --json', { cwd: pkgPath, encoding: 'utf8' }); | |
| const tarballName = JSON.parse(output)[0].filename; |
| execSync(`npm install --no-audit --no-fund ${peerDeps.join(" ")} ${tarballs.join(" ")}`, { | ||
| cwd: testProjDir, | ||
| stdio: "inherit", | ||
| }); |
There was a problem hiding this comment.
If the temporary directory path or any of the package paths contain spaces (which is common in some environments or OS configurations), passing them unquoted to execSync will cause the shell command to fail or behave unexpectedly. Wrapping the arguments in double quotes ensures the command runs successfully regardless of spaces in paths.
| execSync(`npm install --no-audit --no-fund ${peerDeps.join(" ")} ${tarballs.join(" ")}`, { | |
| cwd: testProjDir, | |
| stdio: "inherit", | |
| }); | |
| const installArgs = [...peerDeps, ...tarballs].map(arg => '"' + arg + '"').join(' '); | |
| execSync('npm install --no-audit --no-fund ' + installArgs, { | |
| cwd: testProjDir, | |
| stdio: 'inherit', | |
| }); |
annajowang
force-pushed
the
feat/validate-prod-deps-ci
branch
4 times, most recently
from
5a16421 to
6284b32
Compare
Verify that the built packages can be installed and run (exiting with 0 when called with --help) in a clean environment without devDependencies. This catches cases where devDependencies are accidentally used as production dependencies. Added --help support to nextjs and angular adapter build/create binaries to allow them to exit early with success instead of trying to run a full build/create during verification. BUG=b/466103915 TAG=agy CONV=25a32dbb-c6bf-4394-be10-d693f5f74670
annajowang
force-pushed
the
feat/validate-prod-deps-ci
branch
from
6284b32 to
482805b
Compare
annajowang
removed request for
abhis3,
falahat,
jamesdaniels,
sjjj986 and
taeold
1 participant
Description
This PR introduces a robust package verification step to our CI workflow to ensure that published packages do not have missing production dependencies (which were previously masked by monorepo hoisting).
During implementation, this verification script successfully identified and fixed a pre-existing missing dependency bug in
@apphosting/build.Key Changes
scripts/verify-packages.js):import()and CLI tools by running them with--help.distfolder).isMainGuards):isMain(import.meta)guards to the Next.js and Angular adapter binaries (build.tsandcreate.ts).semver) without triggering their build execution logic.verify_packagesjob to.github/workflows/test.ymlthat runs after thebuildjob.yamldependency topackages/@apphosting/build/package.json(discovered by the new verification script).